Open
Conversation
…arent. On ne peut pas modifier ou supprimer un élément désactivé
…ur il y avait une erreur.
good Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…under a child menu (infinite loop). The drop-down menu no longer displays a sub-menu for added security.
…t checks that its parent category is active before allowing the value to be set to 1
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full menus subsystem: kernel models for categories/items, admin CRUD UI with AJAX and permissions, theme integration for loading/caching and affix rendering (including inbox counts), frontend recursive multilevel menu rendering, JS/CSS assets, language entries, and an upgrade path creating schema and defaults. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant AdminUI as System Admin Module
participant DB as Database
participant Cache as Cache Layer
participant Theme as Theme Engine
participant Frontend as Frontend Renderer
Admin->>AdminUI: create/update/delete category or item
AdminUI->>DB: persist changes & update permissions
DB-->>AdminUI: confirm
AdminUI->>Cache: invalidateMenusCache()
Frontend->>Theme: page init -> loadMenus()
Theme->>Cache: lookup cache (language + groups)
alt cache hit
Cache-->>Theme: return menus
else cache miss
Theme->>DB: query active categories & items with perms
DB-->>Theme: hierarchical menu data
Theme->>Cache: store menus (TTL) & register key
end
Theme->>Theme: renderMenuAffixesRecursive() / normalizeMenuUrl()
Theme->>DB: getInboxUnreadCount() (if needed)
Theme-->>Frontend: assign xoMenuCategories
Frontend->>Frontend: render recursive multilevel menu
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
htdocs/modules/system/constants.php (1)
37-48:⚠️ Potential issue | 🟡 MinorRemove the stale commented constant value to avoid conflicting source-of-truth.
Line 48 still documents
XOOPS_SYSTEM_THEME1as19, but Line 37 now uses19forXOOPS_SYSTEM_MENUS. This comment is now misleading.🧹 Suggested cleanup
define("XOOPS_SYSTEM_THEME1", 18); define("XOOPS_SYSTEM_MENUS", 19); @@ -//define("XOOPS_SYSTEM_THEME1", 19);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/constants.php` around lines 37 - 48, Remove the stale commented constant definition for XOOPS_SYSTEM_THEME1 to avoid confusion with the current XOOPS_SYSTEM_MENUS value; locate the commented line "//define("XOOPS_SYSTEM_THEME1", 19);" in the constants block and delete it so XOOPS_SYSTEM_MENUS remains the single source-of-truth for the value 19.htdocs/modules/system/admin/modulesadmin/modulesadmin.php (1)
587-622:⚠️ Potential issue | 🟠 MajorInconsistency:
xoops_module_uninstallreserved tables list is not updated.The
$reservedTablesarray inxoops_module_install(lines 36-73) now includesmenuscategoryandmenusitems, but the duplicate array inxoops_module_uninstall(lines 587-622) does not. This means module uninstallation won't protect these tables from being dropped.🐛 Add missing tables to uninstall function
'xoopsnotifications', 'banner', 'bannerclient', 'bannerfinish', + 'menuscategory', + 'menusitems', ]; $db = XoopsDatabaseFactory::getDatabaseConnection();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php` around lines 587 - 622, The reserved table list in xoops_module_uninstall is missing the new menu tables; update the $reservedTables array inside the xoops_module_uninstall function to include 'menuscategory' and 'menusitems' (matching the xoops_module_install list), ensuring the uninstall path protects those tables from being dropped; locate the $reservedTables declaration in xoops_module_uninstall and add those two entries.
🧹 Nitpick comments (7)
htdocs/modules/system/language/english/menus/menus.dist.php (1)
6-6: Avoid shipping an empty generic placeholder constant.Line 6 defines
MENUS_as an empty string; this can silently mask missing labels if referenced and doesn’t provide practical value as a template.✂️ Suggested change
-define('MENUS_', '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/language/english/menus/menus.dist.php` at line 6, Remove the empty placeholder constant define('MENUS_', ''); or replace it with a meaningful default label/comment; locate the define for MENUS_ and either delete that line or set it to a sensible string (e.g., a real menu label or a commented example) so missing translations are not silently masked by an empty value.htdocs/modules/system/css/multilevelmenu.css (1)
31-37: Add keyboard-focus fallback for nested submenu visibility.The custom JavaScript only handles click events for
.dropdown-submenu > a; nested submenus cannot open via keyboard navigation. While Bootstrap'sdata-bs-toggle="dropdown"provides keyboard support for primary dropdowns, the custom toggle logic for nested items bypasses that. Adding:focus-withinensures keyboard users can access nested menus when focus enters the submenu container.♿ Suggested CSS fallback
.dropdown-submenu:hover > .dropdown-menu { display: block; } +.dropdown-submenu:focus-within > .dropdown-menu { + display: block; +} /* show when toggled by javascript (touch/mobile and click users) */ .dropdown-submenu > .dropdown-menu.show { display: block; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/css/multilevelmenu.css` around lines 31 - 37, Add a keyboard-focus fallback so nested submenus open when focused: update the CSS that controls nested submenu visibility (selectors referencing .dropdown-submenu and .dropdown-menu) to include a :focus-within rule (e.g., .dropdown-submenu:focus-within > .dropdown-menu) so the submenu is displayed when focus enters the submenu container; ensure this complements the existing hover and .show rules and does not remove them.htdocs/modules/system/css/menus.css (1)
11-17: Minor:cursor: not-allowedhas no effect whenpointer-events: none.When
pointer-events: noneis set, the element doesn't receive pointer events, so the cursor style won't be applied. The cursor will be determined by the element underneath. This is cosmetic-only and doesn't affect functionality.🔧 Optional: Remove redundant cursor rule
.item-active-toggle.disabled, .category-active-toggle.disabled { opacity: 0.5; pointer-events: none; - cursor: not-allowed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/css/menus.css` around lines 11 - 17, The CSS rule for selectors .item-active-toggle.disabled and .category-active-toggle.disabled sets pointer-events: none which prevents cursor styles from applying; remove the redundant "cursor: not-allowed" declaration from that rule (leave opacity: 0.5 and pointer-events: none intact) so the stylesheet is not misleading about an unused property.htdocs/kernel/menusitems.php (2)
119-125: Consider using$GLOBALS['xoopsDB']for consistency.Using
global $xoopsDBworks but$GLOBALS['xoopsDB']is more explicit and aligns with patterns used elsewhere in the codebase (e.g., line 204 uses$GLOBALS['xoops']).♻️ Proposed refactor
public function get_new_enreg() { - global $xoopsDB; - $new_enreg = $xoopsDB->getInsertId(); - - return $new_enreg; + return $GLOBALS['xoopsDB']->getInsertId(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/kernel/menusitems.php` around lines 119 - 125, In the get_new_enreg() method replace the use of the global declaration with the explicit $GLOBALS array: remove "global $xoopsDB;" and reference $GLOBALS['xoopsDB'] when calling getInsertId(); ensure the function still returns the same $new_enreg value and update any local variable name usage if needed so the method reads $new_enreg = $GLOBALS['xoopsDB']->getInsertId(); return $new_enreg; to match the project's $GLOBALS['xoops'] pattern.
197-203: Caution: Mutating object'sitems_titlefor display purposes.The loop modifies each object's
items_titleviasetVar()for display in the select element. If these objects are later persisted elsewhere, the admin-formatted title (e.g., "Home (HOME_LABEL)") would be saved to the database instead of the original value. This is likely safe withingetFormItems()scope, but be aware of this pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/kernel/menusitems.php` around lines 197 - 203, The code mutates each object's items_title by calling setVar('items_title', ...) which can persist an admin-formatted title; instead avoid mutating originals in the foreach over $item_arr: either clone the object (e.g., $clone = clone $obj; $clone->setVar('items_title', $obj->getAdminTitle()); $item_arr[$key] = $clone) or do not call setVar at all and use $label = $obj->getAdminTitle() when building the select options; update the loop to use clone or use a separate labels array so setVar, getAdminTitle, $item_arr and items_title are not applied to the original objects.htdocs/modules/system/js/menus.js (1)
120-121: Minor typo in comment."helper to update og row visuals" should be "helper to update of row visuals" or "helper to update row visuals".
📝 Fix typo
- // helper to update og row visuals depending on state + // helper to update row visuals depending on state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/js/menus.js` around lines 120 - 121, The comment above function updateRowState contains a typo "og" — update the comment to read "helper to update row visuals depending on state" (or "helper to update of row visuals depending on state") so it correctly describes the purpose of the updateRowState($elem, state) helper; simply edit that comment string to the corrected wording.htdocs/modules/system/include/update.php (1)
83-129: Consider consolidating permission inserts.Multiple individual INSERT statements for
group_permissioncould be consolidated into a single multi-value INSERT for better performance and readability. However, this is a one-time migration, so impact is minimal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/include/update.php` around lines 83 - 129, Consolidate the many single-row INSERTs into one multi-row INSERT to improve performance and readability: build a single SQL string using $xoopsDB->prefix('group_permission') with all value tuples for the different permissions (those currently inserted via repeated $sql assignments and $xoopsDB->query calls) and execute it once with $xoopsDB->query; ensure the same column order and include the NULL primary key placeholders and the exact permission names ('menus_category_view' / 'menus_items_view') used in the original statements so the semantics of the inserts performed by update.php are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/theme.php`:
- Around line 687-691: The code decodes the stored suffix into $decodedSuffix
but then returns the original (possibly entity-encoded) $value unless it
contains 'xoInboxCount', causing HTML affixes to render as text; change the
non-placeholder branch to return the decoded value instead of $value — i.e.,
after computing $decodedSuffix (used in stripos($decodedSuffix,
'xoInboxCount')), return $decodedSuffix when no placeholder is found so
icons/markup render correctly (update the branch around $decodedSuffix and the
stripos check in theme.php).
- Around line 479-482: The catch blocks in the Theme class (e.g., in loadMenus()
invoked during xoInit()/affix rendering) currently only catch Exception and
should also catch PHP Errors; replace each "catch (Exception $e)" (including the
blocks that set $category_arr = []; $viewPermissionItem = []; and similar
recovery paths) with "catch (Throwable $e)" (or add a subsequent catch
(\Throwable $e)) so that Errors are handled the same way and the code degrades
to empty menus; ensure the existing recovery behavior (resetting $category_arr,
$viewPermissionItem, etc.) is preserved in those Throwable handlers.
In `@htdocs/kernel/menuscategory.php`:
- Around line 88-111: The docblock above getAdminTitle() was copied from
elsewhere and mentions items_title; update it to accurately describe this
method: reference the actual property name 'category_title' (from
getVar('category_title')), explain that if the stored value is a constant name
the method returns the resolved constant value plus the constant name in
parentheses (e.g., constant('CONST') . ' (CONST)'), otherwise returns the stored
title as-is, and keep the `@return` string tag; ensure the examples and
description reference getAdminTitle() and 'category_title' and not
'items_title'.
- Around line 68-86: Update the docblock for getResolvedTitle to reference the
correct property name: replace any mention of "items_title" in the examples and
description with "category_title" so the examples match the actual field used by
the method; ensure the example lines and description mention category_title and
the method name getResolvedTitle for clarity (this docblock likely copied from
XoopsMenusItems).
In `@htdocs/modules/system/admin/menus/main.php`:
- Around line 161-176: When $menuscategoryHandler->insert($obj) fails the code
only assigns error_message and does not rebuild or reassign the form, so POSTed
values are lost; update the failure branch (where
$menuscategoryHandler->insert($obj) returns false and where similar handling
exists around lines 544-559) to recreate the form object with the current $obj
state and assign it to the template (the same template key used when rendering
the form on GET), then assign $obj->getHtmlErrors() as before so the form
displays submitted values and validation errors; look for the insert call and
the $xoopsTpl->assign('error_message', ...) usages to modify those branches.
- Around line 201-214: The delete sequence currently removes the category
($menuscategoryHandler->delete) before removing child items
($menusitemsHandler->delete) and their permissions
(\Xmf\Module\Helper\Permission->deletePermissionForItem), which can leave
partial state if a later delete fails; change both branches (the category-delete
blocks around the CriteriaCompo/Criteria and child loop, including the similar
block at the other location) to perform: start a DB transaction, delete child
item permissions and items first (iterate items_arr, call
deletePermissionForItem for 'menus_items_view' then $menusitemsHandler->delete),
then delete category permissions (deletePermissionForItem 'menus_category_view')
and finally delete the category ($menuscategoryHandler->delete); commit the
transaction on success and rollback on any failure, ensuring errors propagate so
rollback occurs on exceptions.
- Around line 664-677: The loop is re-walking the same subtree for every
ancestor causing O(n²); fix by preventing duplicate recursion: build a lookup of
items by parent id from $allItems (e.g., group by items_pid) and either (A)
perform a single depth-first traversal starting from the direct children of the
category_id using that in-memory map, or (B) keep a $visited set and before
calling $recursiveUpdate($menusitemsHandler, $idtmp, $new, $updatedItems) check
if $idtmp is already visited and skip if so; update $recursiveUpdate to accept
and use the in-memory children map (or visited set) instead of re-querying
$menusitemsHandler so descendants are processed exactly once and $updatedItems
updated accordingly.
- Around line 492-528: Validate that the requested items_cid refers to an
existing category and prevent splitting subtrees when changing an existing
item's category: before saving, use the categories handler to confirm items_cid
exists (for new items reject/save error if not found); when $id>0, if the
supplied items_cid differs from the current $obj->getVar('items_cid'), either
cascade the change by loading all descendants via $menusitemsHandler->getall(new
Criteria('items_cid', <currentCid>)) and update each descendant's items_cid to
the new value, or reject the operation with an error to forbid moving a parent
without moving its descendants; ensure this check happens alongside the existing
parent/descendant logic that inspects items_pid and items_id so no subtree is
left split.
In `@htdocs/modules/system/include/update.php`:
- Around line 44-58: The self-referencing foreign key on menusItemsTable causes
violations because root rows use items_pid = 0 while items_id starts at 1; fix
by making items_pid nullable and using NULL for roots: change the column
definition in the CREATE TABLE to "items_pid INT NULL DEFAULT NULL" (so the FK
can accept NULL) and keep the FOREIGN KEY (items_pid) REFERENCES
{menusItemsTable}(items_id) ON DELETE CASCADE, then update all INSERTs that
create root items to set items_pid = NULL instead of 0; ensure any code that
relies on 0-as-root (e.g., XoopsObjectTree usage) still treats NULL equivalently
or convert those checks if needed.
In `@htdocs/modules/system/language/english/admin/menus.php`:
- Around line 73-83: The MENUS_* constants (e.g., MENUS_HOME, MENUS_ADMIN,
MENUS_ACCOUNT, MENUS_ACCOUNT_EDIT, MENUS_ACCOUNT_LOGIN, MENUS_ACCOUNT_LOGOUT,
MENUS_ACCOUNT_MESSAGES, MENUS_ACCOUNT_NOTIFICATIONS, MENUS_ACCOUNT_REGISTER,
MENUS_ACCOUNT_TOOLBAR) are defined in the admin-only file but must be available
to the frontend; move their define(...) lines out of the admin/menus.php file
and add them to the frontend-accessible menus/menus.dist.php (menus/menus.php)
so public pages load them; remove the duplicate defines from admin/menus.php (or
leave a comment pointing to the shared file) to avoid redefinition, and keep the
exact constant names/strings when copying so translations and references
continue to work.
In `@htdocs/modules/system/templates/admin/system_menus.tpl`:
- Around line 75-83: Replace the clickable <span> badges used for
category-active-toggle (class "category-active-toggle") and item-active-toggle
(class "item-active-toggle") with semantic <button> elements so they are
keyboard-focusable, preserved styles, and carry the same data attributes
(data-id, data-active); ensure each button includes aria-pressed set to "true"
when active and "false" when inactive, and update the JS that toggles the state
to flip both data-active and aria-pressed in sync when a toggle is clicked or
activated via keyboard.
- Around line 157-159: The delete-item anchor in the template has a stray ">"
inside the href (href="...item_id=<{$item.id|escape}>>") which produces a
malformed URL; remove the extra ">" after the <{$item.id|escape} so the href
ends with item_id=<{$item.id|escape}>", leaving the rest of the anchor (class,
title using smarty.const._AM_SYSTEM_MENUS_DELITEM and the conditional
aria-disabled/tabindex logic) unchanged.
- Around line 5-11: The JS string literals assigned to window.XOOPS_MENUS.labels
and window.XOOPS_MENUS.messages must have their translations escaped for
JavaScript contexts; update each interpolation (e.g.,
_AM_SYSTEM_MENUS_ACTIVE_YES, _AM_SYSTEM_MENUS_ACTIVE_NO,
_AM_SYSTEM_MENUS_ERROR_PARENTINACTIVE) to use Smarty's JS escaping (for example
|escape:'javascript' or an equivalent addslashes/json_encode approach) so quotes
and backslashes are escaped when building window.XOOPS_MENUS.labels and
window.XOOPS_MENUS.messages.
In `@htdocs/modules/system/themes/cpadmin/theme.tpl`:
- Around line 77-85: The username fallback branch doesn't include the text-wrap
class, so when $xoops_name is empty the $xoops_uname link can overflow; add the
same class attribute (class="d-block text-wrap") to the anchor in the else
branch for the $xoops_uname link in theme.tpl (the same element that currently
reads <a href="<{xoAppUrl url='user.php'}>" class="d-block" rel="external">) so
both branches use text-wrap and prevent sidebar overflow.
In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl`:
- Around line 25-35: The template currently always emits an empty primary nav
when $xoMenuCategories is empty; update nav-menu.tpl to guard usage of
$xoMenuCategories (the foreach over $xoMenuCategories and the renderMenu call)
and provide a fallback: either render the original hard-coded navbar markup or
hide the <ul class="navbar-nav me-auto"> when $xoMenuCategories is
empty/undefined, so installs where xos_opal_Theme::loadMenus() returns [] (menus
disabled/not seeded) still show a usable nav; specifically wrap the
foreach/renderMenu logic in a conditional check for non-empty $xoMenuCategories
and insert the fallback menu markup or omit the list accordingly.
---
Outside diff comments:
In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php`:
- Around line 587-622: The reserved table list in xoops_module_uninstall is
missing the new menu tables; update the $reservedTables array inside the
xoops_module_uninstall function to include 'menuscategory' and 'menusitems'
(matching the xoops_module_install list), ensuring the uninstall path protects
those tables from being dropped; locate the $reservedTables declaration in
xoops_module_uninstall and add those two entries.
In `@htdocs/modules/system/constants.php`:
- Around line 37-48: Remove the stale commented constant definition for
XOOPS_SYSTEM_THEME1 to avoid confusion with the current XOOPS_SYSTEM_MENUS
value; locate the commented line "//define("XOOPS_SYSTEM_THEME1", 19);" in the
constants block and delete it so XOOPS_SYSTEM_MENUS remains the single
source-of-truth for the value 19.
---
Nitpick comments:
In `@htdocs/kernel/menusitems.php`:
- Around line 119-125: In the get_new_enreg() method replace the use of the
global declaration with the explicit $GLOBALS array: remove "global $xoopsDB;"
and reference $GLOBALS['xoopsDB'] when calling getInsertId(); ensure the
function still returns the same $new_enreg value and update any local variable
name usage if needed so the method reads $new_enreg =
$GLOBALS['xoopsDB']->getInsertId(); return $new_enreg; to match the project's
$GLOBALS['xoops'] pattern.
- Around line 197-203: The code mutates each object's items_title by calling
setVar('items_title', ...) which can persist an admin-formatted title; instead
avoid mutating originals in the foreach over $item_arr: either clone the object
(e.g., $clone = clone $obj; $clone->setVar('items_title',
$obj->getAdminTitle()); $item_arr[$key] = $clone) or do not call setVar at all
and use $label = $obj->getAdminTitle() when building the select options; update
the loop to use clone or use a separate labels array so setVar, getAdminTitle,
$item_arr and items_title are not applied to the original objects.
In `@htdocs/modules/system/css/menus.css`:
- Around line 11-17: The CSS rule for selectors .item-active-toggle.disabled and
.category-active-toggle.disabled sets pointer-events: none which prevents cursor
styles from applying; remove the redundant "cursor: not-allowed" declaration
from that rule (leave opacity: 0.5 and pointer-events: none intact) so the
stylesheet is not misleading about an unused property.
In `@htdocs/modules/system/css/multilevelmenu.css`:
- Around line 31-37: Add a keyboard-focus fallback so nested submenus open when
focused: update the CSS that controls nested submenu visibility (selectors
referencing .dropdown-submenu and .dropdown-menu) to include a :focus-within
rule (e.g., .dropdown-submenu:focus-within > .dropdown-menu) so the submenu is
displayed when focus enters the submenu container; ensure this complements the
existing hover and .show rules and does not remove them.
In `@htdocs/modules/system/include/update.php`:
- Around line 83-129: Consolidate the many single-row INSERTs into one multi-row
INSERT to improve performance and readability: build a single SQL string using
$xoopsDB->prefix('group_permission') with all value tuples for the different
permissions (those currently inserted via repeated $sql assignments and
$xoopsDB->query calls) and execute it once with $xoopsDB->query; ensure the same
column order and include the NULL primary key placeholders and the exact
permission names ('menus_category_view' / 'menus_items_view') used in the
original statements so the semantics of the inserts performed by update.php are
preserved.
In `@htdocs/modules/system/js/menus.js`:
- Around line 120-121: The comment above function updateRowState contains a typo
"og" — update the comment to read "helper to update row visuals depending on
state" (or "helper to update of row visuals depending on state") so it correctly
describes the purpose of the updateRowState($elem, state) helper; simply edit
that comment string to the corrected wording.
In `@htdocs/modules/system/language/english/menus/menus.dist.php`:
- Line 6: Remove the empty placeholder constant define('MENUS_', ''); or replace
it with a meaningful default label/comment; locate the define for MENUS_ and
either delete that line or set it to a sensible string (e.g., a real menu label
or a commented example) so missing translations are not silently masked by an
empty value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f951e2c-711e-4bee-af12-20181d4cccee
📒 Files selected for processing (26)
htdocs/class/theme.phphtdocs/kernel/menuscategory.phphtdocs/kernel/menusitems.phphtdocs/modules/system/admin/menu.phphtdocs/modules/system/admin/menus/index.phphtdocs/modules/system/admin/menus/main.phphtdocs/modules/system/admin/menus/xoops_version.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phphtdocs/modules/system/constants.phphtdocs/modules/system/css/menus.csshtdocs/modules/system/css/multilevelmenu.csshtdocs/modules/system/include/update.phphtdocs/modules/system/js/menus.jshtdocs/modules/system/js/multilevelmenu.jshtdocs/modules/system/language/english/admin.phphtdocs/modules/system/language/english/admin/menus.phphtdocs/modules/system/language/english/menu.phphtdocs/modules/system/language/english/menus/index.phphtdocs/modules/system/language/english/menus/menus.dist.phphtdocs/modules/system/language/english/modinfo.phphtdocs/modules/system/templates/admin/system_menus.tplhtdocs/modules/system/themes/cpadmin/cpadmin.phphtdocs/modules/system/themes/cpadmin/theme.tplhtdocs/modules/system/xoops_version.phphtdocs/themes/xswatch5/style.csshtdocs/themes/xswatch5/tpl/nav-menu.tpl
There was a problem hiding this comment.
♻️ Duplicate comments (1)
htdocs/class/theme.php (1)
687-691:⚠️ Potential issue | 🟠 MajorReturn the decoded value when no placeholder is found.
The code decodes
$valueinto$decodedSuffixto check for placeholders, but when none are found, it returns the original entity-encoded$value. This causes HTML affixes (like icons) to render as escaped text in the template.🐛 Proposed fix
$decodedSuffix = htmlspecialchars_decode($value, ENT_QUOTES | ENT_HTML5); if (false === stripos($decodedSuffix, 'xoInboxCount')) { - return $value; + return $decodedSuffix; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/theme.php` around lines 687 - 691, The code decodes $value into $decodedSuffix to search for the xoInboxCount placeholder but still returns the original (entity-encoded) $value when no placeholder is found; change the early return to return the decoded string instead. In other words, in the block where you compute $decodedSuffix and call stripos($decodedSuffix, 'xoInboxCount'), replace returning $value with returning $decodedSuffix so icons/HTML affixes render correctly while keeping the existing placeholder-handling logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@htdocs/class/theme.php`:
- Around line 687-691: The code decodes $value into $decodedSuffix to search for
the xoInboxCount placeholder but still returns the original (entity-encoded)
$value when no placeholder is found; change the early return to return the
decoded string instead. In other words, in the block where you compute
$decodedSuffix and call stripos($decodedSuffix, 'xoInboxCount'), replace
returning $value with returning $decodedSuffix so icons/HTML affixes render
correctly while keeping the existing placeholder-handling logic intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
htdocs/class/theme.php (1)
500-501: Moveclass/tree.phpinclude outside the category loop.Line 500 runs inside
foreach ($category_arr as $cat). Even withinclude_once, moving it outside the loop improves clarity and avoids repeated include checks.♻️ Suggested cleanup
- foreach ($category_arr as $cat) { + include_once $GLOBALS['xoops']->path('class/tree.php'); + foreach ($category_arr as $cat) { try { @@ - include_once $GLOBALS['xoops']->path('class/tree.php'); $myTree = new XoopsObjectTree($items_arr, 'items_id', 'items_pid');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/theme.php` around lines 500 - 501, The include_once of class/tree.php is inside the foreach over $category_arr causing repeated include checks; move the include_once $GLOBALS['xoops']->path('class/tree.php') call to before the foreach ($category_arr as $cat) loop so the file is loaded once, and keep the XoopsObjectTree instantiation (new XoopsObjectTree($items_arr, 'items_id', 'items_pid')) where it is so each category still builds its tree.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/theme.php`:
- Around line 667-669: The current preg_match in theme.php returns any absolute
URL scheme (e.g. javascript:, data:) allowing unsafe URIs; update the check so
only allowed schemes are returned — replace the broad preg_match(...) that tests
$url with one that only permits safe schemes (e.g. http, https, mailto) plus
protocol-relative '//' and root/fragment/query ('/','#','?') before returning;
modify the preg_match call (the one that inspects $url) to use an allowlist of
schemes and ensure all other URLs are treated as relative/sanitized instead of
returned raw.
---
Nitpick comments:
In `@htdocs/class/theme.php`:
- Around line 500-501: The include_once of class/tree.php is inside the foreach
over $category_arr causing repeated include checks; move the include_once
$GLOBALS['xoops']->path('class/tree.php') call to before the foreach
($category_arr as $cat) loop so the file is loaded once, and keep the
XoopsObjectTree instantiation (new XoopsObjectTree($items_arr, 'items_id',
'items_pid')) where it is so each category still builds its tree.
…an item (for modified items, not for new items). The likelihood of this causing a problem is very, very low.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
htdocs/modules/system/language/english/admin/menus.php (1)
75-85:⚠️ Potential issue | 🟠 MajorAvoid defining
MENUS_*in the admin language file to prevent constant redefinition.Line 76-85 duplicates constants that are expected in frontend
menus/menus.php(also indicated by the tip on Line 19-20). Because both files are loaded, this can produce “already defined” warnings and inconsistent translation ownership.🛠️ Suggested fix
-// Menus -define('MENUS_HOME', 'Home'); -define('MENUS_ADMIN', 'Administration'); -define('MENUS_ACCOUNT', 'Account'); -define('MENUS_ACCOUNT_EDIT', 'Edit Account'); -define('MENUS_ACCOUNT_LOGIN', 'Login'); -define('MENUS_ACCOUNT_LOGOUT', 'Logout'); -define('MENUS_ACCOUNT_MESSAGES', 'Messages'); -define('MENUS_ACCOUNT_NOTIFICATIONS', 'Notifications'); -define('MENUS_ACCOUNT_REGISTER', 'Sign Up'); -define('MENUS_ACCOUNT_TOOLBAR', 'Toolbar'); +// MENUS_* constants belong in modules/system/language/<lang>/menus/menus.php +// (frontend/shared menu labels)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/language/english/admin/menus.php` around lines 75 - 85, The admin language file is redefining frontend menu constants (MENUS_HOME, MENUS_ADMIN, MENUS_ACCOUNT, MENUS_ACCOUNT_EDIT, MENUS_ACCOUNT_LOGIN, MENUS_ACCOUNT_LOGOUT, MENUS_ACCOUNT_MESSAGES, MENUS_ACCOUNT_NOTIFICATIONS, MENUS_ACCOUNT_REGISTER, MENUS_ACCOUNT_TOOLBAR) which causes duplicate-definition warnings and ownership confusion; remove these MENUS_* defines from this admin file and ensure the canonical definitions remain only in the frontend menus file (menus/menus.php), or wrap them with a guard (if !defined(...)) so the frontend translations retain priority and no redefinition occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/admin/menus/main.php`:
- Around line 666-679: The recursive update lambda ($recursiveUpdate) ignores
handler->insert() failures for descendants which can leave partially updated
trees while the API returns success; modify $recursiveUpdate (and the similar
blocks at lines referenced) to propagate insert() failure by having it return a
boolean (or set a passed-by-ref flag) and stop/mark failure when any child
insert() returns false, then update propagateActiveState(...) to collect and
combine the root toggle result with cascade results so the final JSON success
value reflects both root and all descendant insert outcomes before emitting the
response.
- Around line 494-546: The code sets items_cid from the request unconditionally
which allows forged POSTs to move protected items; change the logic so that when
$isProtected is true you do NOT call $obj->setVar('items_cid', $items_cid) nor
accept items_pid from Request::getInt; instead preserve the original
category/parent from $obj->getVar('items_cid') and $obj->getVar('items_pid') and
skip or reject any client-supplied changes. Concretely, wrap or gate the
assignment and validation around !$isProtected (or explicitly restore
oldCid/oldPid when $isProtected), keeping the existing menuscategoryHandler and
menusitemsHandler validation only for non-protected items.
---
Duplicate comments:
In `@htdocs/modules/system/language/english/admin/menus.php`:
- Around line 75-85: The admin language file is redefining frontend menu
constants (MENUS_HOME, MENUS_ADMIN, MENUS_ACCOUNT, MENUS_ACCOUNT_EDIT,
MENUS_ACCOUNT_LOGIN, MENUS_ACCOUNT_LOGOUT, MENUS_ACCOUNT_MESSAGES,
MENUS_ACCOUNT_NOTIFICATIONS, MENUS_ACCOUNT_REGISTER, MENUS_ACCOUNT_TOOLBAR)
which causes duplicate-definition warnings and ownership confusion; remove these
MENUS_* defines from this admin file and ensure the canonical definitions remain
only in the frontend menus file (menus/menus.php), or wrap them with a guard (if
!defined(...)) so the frontend translations retain priority and no redefinition
occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7701ab2-8d3a-4df5-8a27-3daf7d8e0c34
📒 Files selected for processing (3)
htdocs/kernel/menuscategory.phphtdocs/modules/system/admin/menus/main.phphtdocs/modules/system/language/english/admin/menus.php
Comment on lines
+666
to
+679
| $recursiveUpdate = function ($handler, $parentId, $state, array &$updated) use (&$recursiveUpdate) { | ||
| $crit = new Criteria('items_pid', (int)$parentId); | ||
| $children = $handler->getAll($crit); | ||
| foreach ($children as $child) { | ||
| $cid = $child->getVar('items_id'); | ||
| if ((int)$child->getVar('items_active') !== $state) { | ||
| $child->setVar('items_active', $state); | ||
| if ($handler->insert($child, true)) { | ||
| $updated[] = $cid; | ||
| } | ||
| } | ||
| $recursiveUpdate($handler, $cid, $state, $updated); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Don’t return success when descendant updates fail.
In both toggle cascades, child insert() failures are ignored, but the API can still return success: true. That leaves partially updated trees and stale UI assumptions.
🛠️ Suggested direction
- $recursiveUpdate = function ($handler, $parentId, $state, array &$updated) use (&$recursiveUpdate) {
+ $recursiveUpdate = function ($handler, $parentId, $state, array &$updated) use (&$recursiveUpdate) {
+ $ok = true;
$crit = new Criteria('items_pid', (int)$parentId);
$children = $handler->getAll($crit);
foreach ($children as $child) {
$cid = $child->getVar('items_id');
if ((int)$child->getVar('items_active') !== $state) {
$child->setVar('items_active', $state);
- if ($handler->insert($child, true)) {
+ if ($handler->insert($child, true)) {
$updated[] = $cid;
+ } else {
+ $ok = false;
}
}
- $recursiveUpdate($handler, $cid, $state, $updated);
+ if (!$recursiveUpdate($handler, $cid, $state, $updated)) {
+ $ok = false;
+ }
}
+ return $ok;
};Apply the same pattern to propagateActiveState(...), then compute final success from root + cascade results before emitting JSON.
Also applies to: 683-693, 801-815, 822-831
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/menus/main.php` around lines 666 - 679, The
recursive update lambda ($recursiveUpdate) ignores handler->insert() failures
for descendants which can leave partially updated trees while the API returns
success; modify $recursiveUpdate (and the similar blocks at lines referenced) to
propagate insert() failure by having it return a boolean (or set a passed-by-ref
flag) and stop/mark failure when any child insert() returns false, then update
propagateActiveState(...) to collect and combine the root toggle result with
cascade results so the final JSON success value reflects both root and all
descendant insert outcomes before emitting the response.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
htdocs/modules/system/admin/menus/main.php (2)
673-687:⚠️ Potential issue | 🟠 MajorToggle endpoints can return success after descendant update failures.
Both recursive cascades ignore failed child
insert()calls, but final JSON success depends only on the root save result. This can returnsuccess: truewith partially updated trees.Also applies to: 698-707, 803-817, 824-833
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/menus/main.php` around lines 673 - 687, The recursive cascade ($recursiveUpdate) currently ignores failed $handler->insert($child, true) calls and only the root save determines the final JSON, allowing success responses when descendant updates failed; modify $recursiveUpdate to propagate insert failures by returning a boolean (or setting a passed-by-reference failure flag) and stop or mark failure when any $handler->insert(...) returns false, ensure all call sites (the other similar blocks at lines noted) check this boolean/failure flag and include it in the overall result so the final JSON success is false if any descendant insert failed; reference $recursiveUpdate, $handler->insert, $updated, $seen, items_active and items_id when making the change.
494-507:⚠️ Potential issue | 🟠 MajorProtected items can still be moved across categories via forged POST.
items_cidis assigned from request before the protected-item guard, so server-side protection is bypassable even though the UI field is disabled.🔒 Minimal server-side lock
- $items_cid = Request::getInt('items_cid', 0); - $oldCid = ($id > 0) ? (int)$obj->getVar('items_cid') : 0; - $obj->setVar('items_cid', $items_cid); + $requestedCid = Request::getInt('items_cid', 0); + $oldCid = ($id > 0) ? (int)$obj->getVar('items_cid') : 0; + $items_cid = $isProtected ? $oldCid : $requestedCid; + $obj->setVar('items_cid', $items_cid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/menus/main.php` around lines 494 - 507, The code reads Request::getInt('items_cid') and immediately calls $obj->setVar('items_cid', $items_cid) allowing forged POSTs to move protected items; change the flow to enforce the protected-item guard first: retrieve $items_cid from Request but do NOT call $obj->setVar('items_cid', ...) until after you check the protection flag on the existing object (use $obj->getVar('items_protected') or the existing protection check used elsewhere), and if the item is protected, keep the original category ($oldCid) or reject the change; ensure the validation that verifies the target category (menuscategoryHandler->get) still runs only for allowed moves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/kernel/menusitems.php`:
- Around line 153-156: The code dereferences $category returned by
$menuscategoryHandler->get($category_id) without checking it; guard the lookup
by verifying $category is a valid object before calling
$category->getVar('category_title') and before adding the hidden field. In the
block around menuscategoryHandler->get, check that $category is an object (or
not null) and when it's invalid, handle gracefully (e.g., set a safe default
title, add an error message/redirect, or skip adding the label/hidden input) so
XoopsFormLabel and the items_cid hidden field are only used when $category is
valid.
In `@htdocs/modules/system/include/update.php`:
- Around line 33-130: The migration block creating and populating the menus
tables (referencing $menusCategoryTable, $menusItemsTable) currently ignores
$xoopsDB->query() return values; change each CREATE TABLE and each INSERT so you
check the result of $xoopsDB->query($sql) and fail fast on error (log or
throw/return false and stop the update) instead of continuing; apply the same
pattern used elsewhere for checking query results (e.g., validate result, use
$xoopsDB->isResultSet/$xoopsDB->getRowsNum or $xoopsDB->errno()/error message)
for every query in this block to ensure the update aborts on the first SQL
failure.
---
Duplicate comments:
In `@htdocs/modules/system/admin/menus/main.php`:
- Around line 673-687: The recursive cascade ($recursiveUpdate) currently
ignores failed $handler->insert($child, true) calls and only the root save
determines the final JSON, allowing success responses when descendant updates
failed; modify $recursiveUpdate to propagate insert failures by returning a
boolean (or setting a passed-by-reference failure flag) and stop or mark failure
when any $handler->insert(...) returns false, ensure all call sites (the other
similar blocks at lines noted) check this boolean/failure flag and include it in
the overall result so the final JSON success is false if any descendant insert
failed; reference $recursiveUpdate, $handler->insert, $updated, $seen,
items_active and items_id when making the change.
- Around line 494-507: The code reads Request::getInt('items_cid') and
immediately calls $obj->setVar('items_cid', $items_cid) allowing forged POSTs to
move protected items; change the flow to enforce the protected-item guard first:
retrieve $items_cid from Request but do NOT call $obj->setVar('items_cid', ...)
until after you check the protection flag on the existing object (use
$obj->getVar('items_protected') or the existing protection check used
elsewhere), and if the item is protected, keep the original category ($oldCid)
or reject the change; ensure the validation that verifies the target category
(menuscategoryHandler->get) still runs only for allowed moves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a678cc3-08ad-4c94-bcf8-ed8bfb1cb135
📒 Files selected for processing (4)
htdocs/class/tree.phphtdocs/kernel/menusitems.phphtdocs/modules/system/admin/menus/main.phphtdocs/modules/system/include/update.php
fix: Use buttons for the active-state toggles. fix: Remove the stray > from the delete-item URL.
ForXoops
approved these changes
Mar 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improvements to the menu system following the previous PR
Summary by CodeRabbit
New Features
Chores
Style/Assets
Bug Fixes